ESA Euclid EUCLIDSWRQ-215 get_datalinks_metadata #3438
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3438 +/- ##
=======================================
Coverage 70.72% 70.72%
=======================================
Files 232 232
Lines 20041 20045 +4
=======================================
+ Hits 14174 14177 +3
- Misses 5867 5868 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great, thanks @jsaizsantos ! Please let us know if you have further comments, @bsipocz . We are preparing for an internal release at the end of this month, so it would be great if we could have this reviewed and merged by next week. |
There was a problem hiding this comment.
Thanks @jsaizsantos for the PR and welcome to astroquery!
I have some comment for the content here, namely to consider not having the separate method just the new kwarg to get_datalinks. And maybe calling it something else then option, maybe extra_options (I'm not really sold on this idea) or something else that best describes it.
| options=options, | ||
| verbose=verbose) | ||
|
|
||
| def get_datalinks_metadata(self, ids, *, linking_parameter='SOURCE_ID', verbose=False): |
There was a problem hiding this comment.
Why have this as a separate method rather than just documenting the one needs to have use options='METADATA'? The narrative documentation example below throw me off the track as starting with that one, and based on the method name I expected only metadata rather than it having the same result as using get_datalinks and some extra metadata.
astroquery/esa/euclid/core.py
Outdated
| options : str, optional, default None | ||
| To let customize the server behaviour |
There was a problem hiding this comment.
options feels very generic; could we be more specific naming this?
Also, I would find it useful to mention what valid values could be for this, from the example below 'METADATA' is one, but how could the user know what else works?
There was a problem hiding this comment.
In addition, adding a new kwarg is an API change, thus will need to be mentioned in the changelog
astroquery/utils/tap/core.py
Outdated
| return user.startswith(f"{user_id}:") and user.count("\\n") == 0 | ||
|
|
||
| def get_datalinks(self, ids, *, linking_parameter=None, verbose=False): | ||
| def get_datalinks(self, ids, *, linking_parameter=None, options=None, verbose=False): |
There was a problem hiding this comment.
This will need to be mentioned in the changelog
There was a problem hiding this comment.
Dear @bsipocz, thanks for your comments! I try to explain:
The best option for me, ideally, would have been to just add a new method without changing the signature of the parent get_datalinks. However, in order to reuse code and avoid copy-paste, I called the existing parent get_datalinks method from the new get_datalinks_metadata, and for getting the new behavior from the server, we needed a new argument to that method of the parent TapPlus class.
You say with good reason that this argument name is generic, and it is on purpose: the idea is that it could be reused in future cases to customize the behavior of the server (as there are several classes extending TapPlus, for different ESA archives, every one having their requirements) without needing to touch this get_datalinks method once and again, as it happened this time. The value of the options argument would tell the exact additional behavior.
I will tackle your suggestions:
- Rename
optionstoextra_options. - Indicate the only valid value for it right now, which is
METADATAfor theEuclidarchive. - Mention the change of signature in the changelog.
|
Dear @bsipocz: |
3 participants
Dear Astroquery community:
This is my first PR to this repository. I am a developer working for the Euclid archive at ESAC.
The purpose of this PR is to add a new method
get_datalinks_metadatato theEuclidclass, to return additional columns on top of the ones returned by the plainget_datalinksmethod, which is left untouched as it complies with the IVOA standard.The actual new columns to return are left to the datalinks service of the Euclid archive; currently it is
datalabs_path, to be used for Datalabs, but two more will come in a new release of the archive (with no further required change on the Astroquery interface):file_nameandhdu_index.cc @esdc-esac-esa-int